-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some adaptations to workflows #9994
base: main
Are you sure you want to change the base?
Some adaptations to workflows #9994
Conversation
e38fc26
to
16d8860
Compare
CodSpeed Performance ReportMerging #9994 will not alter performanceComparing Summary
|
a1fc9a1
to
ab916da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have some minor questions.
src/ert/cli/workflow.py
Outdated
runner = WorkflowRunner( | ||
workflow=workflow, | ||
storage=storage, | ||
ert_config=ert_config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should remove this as it is not available in run models, where most of these are run from?
if is_using_wf_args_fixture | ||
else [*workflow_args, *fixture_args] | ||
) | ||
if not positional_args and not use_kwargs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This we could keep this as it used to be? I dont think kwargs were really supported before, they just ended up being workflow_args
in a list?
src/ert/workflow_runner.py
Outdated
@@ -107,14 +107,16 @@ class WorkflowRunner: | |||
def __init__( | |||
self, | |||
workflow: Workflow, | |||
config_file: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also commented in semeio, could potentially give reports dir, we do something similar for the update report, that has a folder it writes to.
src/ert/workflow_runner.py
Outdated
storage: Storage | None = None, | ||
ensemble: Ensemble | None = None, | ||
ert_config: ErtConfig | None = None, | ||
) -> None: | ||
self.__workflow = workflow | ||
self.storage = storage | ||
self.ensemble = ensemble | ||
self.ert_config = ert_config | ||
self.ert_config = ert_config # Should eventually be removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove now?
a60c42c
to
6e47da3
Compare
87c7ada
to
0aeee68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good :) See some minor questions 🔍
0aeee68
to
9d1a66e
Compare
There seems to be a GUI test hanging now 🤔 |
b828aa7
to
f094ca8
Compare
Co-authored-by: Jonathan Karlsen <[email protected]>
Co-authored-by: Jonathan Karlsen <[email protected]>
Co-authored-by: Jonathan Karlsen <[email protected]>
f094ca8
to
1e668d0
Compare
Issue
Resolves equinor/semeio#685
Add support for defaulted kwargs from workflows